test(shareReplay): don't restart when refCount if the source has completed #5457
Conversation
| const shared = source.pipe(shareReplay({ bufferSize: 1, refCount: true })); | ||
|
|
||
| expectObservable(shared, sub1).toBe(expected1); | ||
| expectObservable(shared, sub2).toBe(expected2); |
There was a problem hiding this comment.
We also need to test that there is only one subscription to the source. That can be done with expectSubscriptions which takes an expected subscription marble diagram - or an array of diagrams if multiple subscriptions are expected.
Something like this:
const source = cold('a-(b|) ');
const sourceSubs = '^-! ';
/* ... */
expectSubscriptions(source.subscriptions).toBe(sourceSubs);If multiple subscriptions to the source are made, this expectation would fail, as sourceSubs would have to be an array of subscriptions.
There was a problem hiding this comment.
Yep. That makes a lot of sense. I've amended that commit with this expectation, thanks!
I've also added another commit that does the same thing on other tests that were not doing that. I do understand that for those tests that expectation may not be as important, because the current expectations are "implicitly" checking for that already. So, I don't mind dropping this commit from the PR. I thought that maybe for consistency it could be worth it to have those expectations?
josepot
force-pushed
the
test/sharereplay-refcount
branch
2 times, most recently
from
13fcd67
to
2863c9d
Compare
|
What's not immediately clear to me from this PR is whether or not this is a breaking change. It doesn't look like it is, but the only way to know that for sure would have been to land the passing test, then land the change proving it didn't break the test. |
|
@benlesh There should be no behavioural changes in this whatsoever. The PR adds a test that describes behaviour that was not fully characterised by the docs. Namely that the completion should be replayed regardless of the The original PR - which was closed - sought to change the behaviour. |
|
@josepot Maybe you should remove the commit that removes the |
josepot
force-pushed
the
test/sharereplay-refcount
branch
from
2863c9d
to
f771c8b
Compare
|
Yep, no worries. I've dropped the commit that removes the |
|
@benlesh I merged this, as the code modification - the removal of the unnecessary variable - was removed, so this PR now only adds a test and some some additional expectations. |
|
@josepot Feel free to open another PR that removes that unnecessary variable. If it's not needed and the code's simpler without it, it might as well be deleted. |
Description:
As discussed in #5455: it would be useful to have a test that removes the ambiguity on what should happen with the
shareReplayoperator ifrefCountistrueonce the source completes.I've also added a second commit that gets rid of the redundant variable
isComplete. This variable became unnecessary after this change. I guess that this second commit is a "nit pick", so I don't mind removing it from this PR. Although, it may marginally reduce the bundle size?Related issue
#5455